Introduce abstraction for error display callbacks. #5484
Conversation
That looks right to me, but we better discuss/mention this on internals@. |
Simplify soap_error_cb logic to be only display callback related, without the requireent to call the previous error handler as before. bug70469.phpt needed to use a better example: error_get_last will now work with SOAPFault because php_error_cb is not overwritten anymore, but the original display logic is never called. So change assertion not to render error.
|
@beberlei I like the idea a lot, but I do wonder how used these options are in reality and what benefit they provide (besides formatting uncatchable errors) vs userland implementations. If @cmb69's RFC for In a way I think potentially |
|
@KalleZ xdebug adds their own display logic for example and soap technically has its own as well. Especially soap and xdebug interact in dangerous ways and Derick had to add specific logic to xdebug to make it work. This change is mostly aimed at solving the third party overwriting of error_cb. |
|
@derickr @morrisonlevi or anyone have comments on the API? |
| zend_error_display_cb display_callback; | ||
| } zend_error_display_callback; | ||
|
|
||
| char *zend_error_get_type_string(int type); |
There was a problem hiding this comment.
Also, the return type of this one can be const char *
| @@ -47,4 +47,18 @@ | |||
|
|
|||
| #define E_HAS_ONLY_FATAL_ERRORS(mask) !((mask) & ~E_FATAL_ERRORS) | |||
|
|
|||
| typedef int (*zend_error_display_cb)(int type, const char *error_filename, const uint32_t error_lineno, char *buffer, int buffer_len); | |||
There was a problem hiding this comment.
Either this should return zend_bool, or it should return SUCCESS/FAILURE.
There was a problem hiding this comment.
buffer_len should probably be size_t?
| BEGIN_EXTERN_C() | ||
| typedef struct { | ||
| zend_error_display_cb display_callback; | ||
| } zend_error_display_callback; |
There was a problem hiding this comment.
Do you have extra fields planned here? Otherwise the nesting is unnecessary.
| zend_error_display_cb display_callback; | ||
| } zend_error_display_callback; | ||
|
|
||
| char *zend_error_get_type_string(int type); |
There was a problem hiding this comment.
Also, the return type of this one can be const char *
| php_printf("<?xml version=\"1.0\"?><methodResponse><fault><value><struct><member><name>faultCode</name><value><int>" ZEND_LONG_FMT "</int></value></member><member><name>faultString</name><value><string>%s:%s in %s on line %" PRIu32 "</string></value></member></struct></value></fault></methodResponse>", PG(xmlrpc_error_number), error_type_str, buffer, error_filename, error_lineno); | ||
| } else { | ||
| if (zend_error_display_handle(type, error_filename, error_lineno, buffer, buffer_len) == 0) { | ||
| // default error handling if no display callback was triggered |
There was a problem hiding this comment.
Shouldn't there always be at least the default callback?
There was a problem hiding this comment.
Nothing wrong with defensive coding :-) (Instead of == 0 at the end, I would have use if (!zend_error_display_handle....
|
I've no issues with this API, but it also doesn't solve anything for me. In Xdebug I override the whole zend_error_cb with my own. |
Okay, I suspected as much... I think this API is a great match for the xmlrpc use-case, but the soap changes look somewhat dubious to me (I would have to look more closely to be sure, but I believe the intention there is to overwrite a lot more than just the display logic), and it's not super clear to me who else is going to be using this API. |
|
Closing this for now. |
Follow up to #4555 - this replaces the hardcoded display logic for errors in main/main.c
php_error_cbwith a list of callbacks that are iterated from the tail to the head. The first callback to render the error returns 1 and all further callbacks are skipped then.Todo
xmlrpc_errorsandxmlrpc_error_numberINI settings from core into ext/xmlrpc. While this is a BC break to require the extension, this feature was added in 2001 and never changed afterwards, especially it also has no tests. Google and DDG search could lead to the assumption that nobody uses it./cc @cmb69 @derickr